Require discoverable credentials and user verification in README examples#501
Open
ttanimichi wants to merge 2 commits into
Open
Require discoverable credentials and user verification in README examples#501ttanimichi wants to merge 2 commits into
ttanimichi wants to merge 2 commits into
Conversation
Most modern platform authenticators and passkey managers already support both Discoverable Credentials and user verification. Wouldn't explicitly requiring them in options_for_create lead to a simpler and more straightforward authentication flow?
ttanimichi
force-pushed
the
authenticator-selection
branch
from
503f034 to
db3fa32
Compare
ttanimichi
changed the title
authenticator_selection to options_for_create examples
santiagorodriguez96
left a comment
santiagorodriguez96
left a comment
Contributor
There was a problem hiding this comment.
Thanks, @ttanimichi! Improving the README is something I've had in mind for a while, and there's definitely room for it – for now, I think it makes sense to include examples of how to set the user_verification and resident_key options. However, there's one thing I'd like to sort out before merging.
The README walkthrough is meant as an example of how to use the gem, but it's easy for it to be read as a reference implementation – so the option values it shows can end up being treated as recommended defaults. My concern is that requiring resident_key and user_verification in the walkthrough could steer someone using the gem for implementing a 2FA flow toward a more constrained setup than they need:
resident_key: "required"shrinks the set of usable authenticators – any that can't create a discoverable credential are excluded from selection – and uses up the limited discoverable-credential storage on roaming keys that do support it, so a key that's already full fails the registration. 2FA needs none of this, since you already have an allow-list.user_verification: "required"likewise excludes authenticators that can't perform user verification, and forces a PIN/biometric even though the credential is already the second factor;"discouraged"(or"preferred") is often the better fit there.
Rather than bake one profile in as the default, could we keep your additions but annotate the decision points, so the example serves both 2FA and passwordless without prescribing either? Left inline suggestions for that.
Comment on lines
+179
to
+183
| exclude: user.credentials.map { |c| c.webauthn_id }, | ||
| authenticator_selection: { | ||
| resident_key: "required", | ||
| user_verification: "required" | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| exclude: user.credentials.map { |c| c.webauthn_id }, | |
| authenticator_selection: { | |
| resident_key: "required", | |
| user_verification: "required" | |
| } | |
| exclude: user.credentials.map { |c| c.webauthn_id }, | |
| authenticator_selection: { | |
| resident_key: "discouraged", # For a passwordless login or 2FA. Use "required" for a passkey-based (passwordless and usernameless) login. | |
| user_verification: "required" # For a passwordless or passkey-based (passwordless and usernameless) login. Use "discouraged" for 2FA. | |
| } |
|
|
||
| begin | ||
| webauthn_credential.verify(session[:creation_challenge]) | ||
| webauthn_credential.verify(session[:creation_challenge], user_verification: true) |
Contributor
There was a problem hiding this comment.
Suggested change
| webauthn_credential.verify(session[:creation_challenge], user_verification: true) | |
| # Enforce user verification (pairs with the "required" request above). Omit for 2FA. | |
| webauthn_credential.verify(session[:creation_challenge], user_verification: true) |
Comment on lines
+231
to
+234
| options = WebAuthn::Credential.options_for_get( | ||
| allow: user.credentials.map { |c| c.webauthn_id }, | ||
| user_verification: "required" | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggested change
| options = WebAuthn::Credential.options_for_get( | |
| allow: user.credentials.map { |c| c.webauthn_id }, | |
| user_verification: "required" | |
| ) | |
| options = WebAuthn::Credential.options_for_get( | |
| # Pass `allow` when the user is already known (passwordless/2FA/reauth). | |
| # For a passkey-based (usernameless and passwordless) login, omit it and resolve the user from `user_handle` after `from_get`. | |
| allow: user.credentials.map { |c| c.webauthn_id }, | |
| user_verification: "required" # For a passwordless or passkey-based (passwordless and usernameless) login. Use "discouraged" for 2FA. | |
| ) |
Comment on lines
+270
to
+271
| sign_count: stored_credential.sign_count, | ||
| user_verification: true |
Contributor
There was a problem hiding this comment.
Suggested change
| sign_count: stored_credential.sign_count, | |
| user_verification: true | |
| sign_count: stored_credential.sign_count, | |
| user_verification: true # For a passwordless or passkey-based (passwordless and usernameless) login. Omit for 2FA. |
Comment on lines
+301
to
+305
| extensions: { appidExclude: domain.to_s }, | ||
| authenticator_selection: { | ||
| resident_key: "required", | ||
| user_verification: "required" | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| extensions: { appidExclude: domain.to_s }, | |
| authenticator_selection: { | |
| resident_key: "required", | |
| user_verification: "required" | |
| } | |
| extensions: { appidExclude: domain.to_s }, | |
| authenticator_selection: { | |
| resident_key: "discouraged", # For a passwordless login or 2FA. Use "required" for a passkey-based (passwordless and usernameless) login. | |
| user_verification: "required" # For a passwordless or passkey-based (passwordless and usernameless) login. Use "discouraged" for 2FA. | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Most modern platform authenticators and passkey managers already support both discoverable credentials and user verification. This PR updates the README walkthrough examples to require them consistently across both the registration and authentication flows.